docs: Clarify that callback can be called more than once #1727
Conversation
real-or-random
added
user-documentation
|
concept ack |
|
@stratospher Want to review this? :) |
There was a problem hiding this comment.
ACK d279379. verified that illegal callback can be called more than once. and the simple route makes sense.
include/secp256k1.h
Outdated
| * the case that this code itself is broken. | ||
| * the case that this code itself is broken. On the other hand, during debug | ||
| * stage, one would want to be informed about such mistakes, and the default | ||
| * (crashing) may be inadvisable. |
There was a problem hiding this comment.
- When this callback is triggered, the API function called is guaranteed not
- to cause a crash, though its return value and output arguments are
- undefined.
when I initially read the comments on master, I assumed "API not crashing guarantee" is only for debug stage (no abort call) since they were in 1 para. but after seeing this diff just realised that crashing in a callback is acceptable and still a stable API regardless of debug/production build. so I think the comments made it clearer for me!
There was a problem hiding this comment.
Nice, paragraph breaks matter. :)
Though I think it is relatively clear even on master. When the callback calls abort, then the entire program will crash immediately. And from that point on, any guarantee you get from the API is vacuous.
There was a problem hiding this comment.
ok that makes sense. but now I'm confused. It's just a line break difference but I interpreted the meaning before and after as:
- before:
* The philosophy is that these shouldn't be dealt with through a
* specific return value, as calling code should not have branches to deal with
* the case that this code itself is broken.
*
* On the other hand, during debug stage, one would want to be informed about
* such mistakes, and the default (crashing) may be inadvisable.
* When this callback is triggered, the API function called is guaranteed not
* to cause a crash, though its return value and output arguments are
API function called is guaranteed not to cause a crash in debug stage (in tests where it's mostly the callback function counting_callback_fn that just counts and no abort)
- Now:
* The philosophy is that these shouldn't be dealt with through a
* specific return value, as calling code should not have branches to deal with
* the case that this code itself is broken. On the other hand, during debug
* stage, one would want to be informed about such mistakes, and the default
* (crashing) may be inadvisable.
*
* When this callback is triggered, the API function called is guaranteed not
* to cause a crash, though its return value and output arguments are
* undefined. A single API call may trigger the callback more than once.
API function called is guaranteed not to cause a crash in any stage (tests or IRL)
=> because it's the callback doing the crash + handling invalid user inputs + user responsibility and so API is stable?
do we want 1 or 2?
There was a problem hiding this comment.
Huh, I think the guarantee we want to provide is:
"An API function called is guaranteed not to crash after the callback returns."
This guarantee holds no matter what the callback function is set to, which corresponds to what you called "in any "stage". Now, of course, this guarantee doesn't say anything if the callback itself causes a crash (and the default callback does) because then the callback never returns, so there's no "after the callback returns".
Now that I've written this, I'm not sure what this actually means. In simple cases, the only thing the API function does after the callback returns is to return 0. This shouldn't crash. But there are more complex cases:
- Some functions try to read more arguments from memory, e.g.,
secp256k1_ec_pubkey_cmptries to read the memory atpubkey1even after an invalidpubkey0triggered the callback. This may crash ifpubkey1points to an invalid memory region. - Some functions write to their output arguments after the callback has returned (see Initialization of output args #1736 for another rabbit hole...).
Perhaps what we should want to say here is something like this: "An API function called is guaranteed not to crash after the callback returns (unless the crash is unrelated to the violation that triggered the callback)". But this is imprecise and ugly.
Perhaps we should drop this sentence entirely. It creates more confusion than it resolves. What about this?
"Should the callback return, the return value and output arguments of the API function call are undefined. Moreover, the same API call may trigger the callback again in this case."
Note that all of this is about API guarantees, so the audience is API users. As a result, "debug stage" means "debug stage of a program using libsecp256k1". (And as a side remark, note that the callbacks can be set at runtime, so strictly speaking, this is not (necessarily) about debug vs. release builds.) So if you develop a program (an application or another library) that uses the libsecp256k1 API, you may want to set a callback that does not crash for debugging purposes. I'm not entirely sure how useful this functionality is to API users, but this is what the docs here describe.
In practice, where this is useful is exactly in our internal tests, as you correctly point out. But what I'm trying to say is that these are our internal tests, where we may do everything because we control the implementation of the API functions. So we could also have an internal way of setting a counting callback that is not exposed through the public API.
If you ask me, the main purpose of setting callback functions is that you can control how the library crashes in production code. Maybe in your application, you don't want abort() but a more graceful termination. Or you can't use the default callback because it writes an error message to stdout but you don't have stdout because you're developing for a hardware wallet. (In the latter case, setting callbacks at runtime won't help you because the program won't compile in the first place. We've added compile-time overrides for this case.)
And on a last note, I don't think all of this is optimally designed. I believe we'd do some things differently if we had a chance to start from scratch.
There was a problem hiding this comment.
Thank you for the awesome explanation! This helped me understand what the API is actually guaranteeing.
"Should the callback return, the return value and output arguments of the API function call are undefined. Moreover, the same API call may trigger the callback again in this case."
I like this wording better!
There was a problem hiding this comment.
Ok nice, force-pushed to switch to (a variant of) this.
|
Concept ACK While working on #1698, I was very confused when I started to see errors stating the illegal callback had been called more than once. Having some documentation to indicated what is allowed/expected would have been very helpful. Haven't had a chance to dig into the exact wording in this PR but generally agree we should have something. |
|
Concept ACK |
real-or-random
force-pushed
the
202508-illegal-twice
branch
from
d279379 to
4d90585
Compare
|
ACK 4d90585. |
a44a339384 Merge bitcoin-core/secp256k1#1750: ci: Use clang-snapshot in "MSan" job 53585f93b7 ci: Use clang-snapshot in "MSan" job 6894c964f3 Fix Clang 21+ `-Wuninitialized-const-pointer` warning when using MSan 2b7337f63a Merge bitcoin-core/secp256k1#1756: ci: Fix image caching and apply other improvements f163c35897 ci: Set `DEBIAN_FRONTEND=noninteractive` 70ae177ca0 ci: Bump `docker/build-push-action` version b2a95a420f ci: Drop `tags` input for `docker/build-push-action` 122014edb3 ci: Add `scope` parameter to `cache-{to,from}` options baa265429f Merge bitcoin-core/secp256k1#1727: docs: Clarify that callback can be called more than once 4d90585fea docs: Improve API docs of _context_set_illegal_callback 895f53d1cf docs: Clarify that callback can be called more than once de6af6ae35 Merge bitcoin-core/secp256k1#1748: bench: improve context creation in ECDH benchmark 5817885153 Merge bitcoin-core/secp256k1#1749: build: Fix warnings in x86_64 assembly check ab560078aa build: Fix warnings in x86_64 assembly check 10dab907e7 Merge bitcoin-core/secp256k1#1741: doc: clarify API doc of `secp256k1_ecdsa_recover` return value dfe284ed2d bench: improve context creation in ECDH benchmark 7321bdf27b doc: clarify API doc of `secp256k1_ecdsa_recover` return value b475654302 Merge bitcoin-core/secp256k1#1745: test: introduce group order byte-array constant for deduplication 0c91c56041 test: introduce group order byte-array constant for deduplication 88be4e8d86 Merge bitcoin-core/secp256k1#1735: musig: Invalidate secnonce in secp256k1_musig_partial_sign 36e76952cb Merge bitcoin-core/secp256k1#1738: check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so) 399b582a5f Split memclear into two versions 4985ac0f89 Merge bitcoin-core/secp256k1#1737: doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static) 7ebaa134a7 check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so) 806de38bfc doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static) 03fb60ad2e Merge bitcoin-core/secp256k1#1681: doc: Recommend clang-cl when building on Windows d93380fb35 Merge bitcoin-core/secp256k1#1731: schnorrsig: Securely clear buf containing k or its negation 8113671f80 Merge bitcoin-core/secp256k1#1729: hash: Use size_t instead of int for RFC6979 outlen copy 325d65a8cf Rename and clear var containing k or -k 960ba5f9c6 Use size_t instead of int for RFC6979 outlen copy 737912430d ci: Add more tests for clang-cl 7379a5bed3 doc: Recommend clang-cl when building on Windows f36afb8b3d Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification 5153cf1c91 tests: refactor tagged hash tests d2dcf52091 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper 489a43d1bf docs: fix broken link to eprint cache.pdf paper d599714147 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report 0458def51e doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations 1aecce5936 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations 106a7cbf41 doc: Exclude modules' `bench_impl.h` headers from coverage report a9e955d3ea autotools, docs: Adjust help string for `--enable-coverage` option e523e4f90e Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment 24ba8ff168 chore(ci): Fix typo in Dockerfile comment 74b8068c5d Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors c25c3c8a88 test: update wycheproof test vectors 20e3b44746 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS` 2c076d907a Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof 7b07b22957 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS 5433648ca0 Fix typos and spellings 9ea54c69b7 tests: update Wycheproof files git-subtree-dir: src/secp256k1 git-subtree-split: a44a339384e1e4b1c0ed7fa59e2857b057f075bf
d543c0d917 Merge bitcoin-core/secp256k1#1734: Introduce (mini) unit test framework f44c1ebd96 Merge bitcoin-core/secp256k1#1719: ci: DRY workflow using anchors a44a339384 Merge bitcoin-core/secp256k1#1750: ci: Use clang-snapshot in "MSan" job 15d014804e ci: Drop default for `inputs.command` in `run-in-docker-action` 1decc49a1f ci: Use YAML anchor and aliases for repeated "CI script" steps dff1bc107d ci, refactor: Generalize use of `matrix.configuration.env_vars` 4b644da199 ci: Use YAML anchor and aliases for repeated "Print logs" steps a889cd93df ci: Bump `actions/checkout` version 574c2f3080 ci: Use YAML anchor and aliases for repeated "Checkout" steps 53585f93b7 ci: Use clang-snapshot in "MSan" job 6894c964f3 Fix Clang 21+ `-Wuninitialized-const-pointer` warning when using MSan 2b7337f63a Merge bitcoin-core/secp256k1#1756: ci: Fix image caching and apply other improvements f163c35897 ci: Set `DEBIAN_FRONTEND=noninteractive` 70ae177ca0 ci: Bump `docker/build-push-action` version b2a95a420f ci: Drop `tags` input for `docker/build-push-action` 122014edb3 ci: Add `scope` parameter to `cache-{to,from}` options 2f4546ce56 test: add --log option to display tests execution 95b9953ea4 test: Add option to display all available tests 953f7b0088 test: support running specific tests/modules targets 0302c1a3d7 test: add --help for command-line options 9ec3bfe22d test: adapt modules to the new test infrastructure 48789dafc2 test: introduce (mini) unit test framework baa265429f Merge bitcoin-core/secp256k1#1727: docs: Clarify that callback can be called more than once 4d90585fea docs: Improve API docs of _context_set_illegal_callback 895f53d1cf docs: Clarify that callback can be called more than once de6af6ae35 Merge bitcoin-core/secp256k1#1748: bench: improve context creation in ECDH benchmark 5817885153 Merge bitcoin-core/secp256k1#1749: build: Fix warnings in x86_64 assembly check ab560078aa build: Fix warnings in x86_64 assembly check 10dab907e7 Merge bitcoin-core/secp256k1#1741: doc: clarify API doc of `secp256k1_ecdsa_recover` return value dfe284ed2d bench: improve context creation in ECDH benchmark 7321bdf27b doc: clarify API doc of `secp256k1_ecdsa_recover` return value b475654302 Merge bitcoin-core/secp256k1#1745: test: introduce group order byte-array constant for deduplication 9cce703863 refactor: move 'gettime_i64()' to tests_common.h 0c91c56041 test: introduce group order byte-array constant for deduplication 88be4e8d86 Merge bitcoin-core/secp256k1#1735: musig: Invalidate secnonce in secp256k1_musig_partial_sign 36e76952cb Merge bitcoin-core/secp256k1#1738: check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so) 399b582a5f Split memclear into two versions 4985ac0f89 Merge bitcoin-core/secp256k1#1737: doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static) 7ebaa134a7 check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so) 806de38bfc doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static) 03fb60ad2e Merge bitcoin-core/secp256k1#1681: doc: Recommend clang-cl when building on Windows d93380fb35 Merge bitcoin-core/secp256k1#1731: schnorrsig: Securely clear buf containing k or its negation 8113671f80 Merge bitcoin-core/secp256k1#1729: hash: Use size_t instead of int for RFC6979 outlen copy 325d65a8cf Rename and clear var containing k or -k 960ba5f9c6 Use size_t instead of int for RFC6979 outlen copy 737912430d ci: Add more tests for clang-cl 7379a5bed3 doc: Recommend clang-cl when building on Windows f36afb8b3d Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification 5153cf1c91 tests: refactor tagged hash tests d2dcf52091 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper 489a43d1bf docs: fix broken link to eprint cache.pdf paper d599714147 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report 0458def51e doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations 1aecce5936 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations 106a7cbf41 doc: Exclude modules' `bench_impl.h` headers from coverage report a9e955d3ea autotools, docs: Adjust help string for `--enable-coverage` option e523e4f90e Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment 24ba8ff168 chore(ci): Fix typo in Dockerfile comment 74b8068c5d Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors c25c3c8a88 test: update wycheproof test vectors 20e3b44746 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS` 2c076d907a Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof 7b07b22957 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS 5433648ca0 Fix typos and spellings 9ea54c69b7 tests: update Wycheproof files git-subtree-dir: src/secp256k1 git-subtree-split: d543c0d917a76a201578948701cc30ef336e0fe6
5 participants
The tests in #1698 reminded me that some functions, e.g.,
secp256k1_ec_pubkey_cmp, may call the illegal callback more than once (see #1390 (comment) for more context). This PR clarifies the API docs to state explicitly that this is possible.This is the simplest solution. Any production code should crash anyway if it encounters a callback. And in debug code or in our test code, it doesn't really matter whether you see an error message once or twice.
The alternative is to provide a guarantee that the callback is called only once. But that would make our code more complex for no good reason.
The second commit fixes a few typos, wording, and consistency.